fix attachment file size limit in server backend #35519

a1012112796 · 2025-09-22T14:01:03Z

fix go-gitea#35512 Signed-off-by: a1012112796 <1012112796@qq.com>

techknowlogick · 2025-09-22T14:51:22Z

CI fail is related:


--- FAIL: TestCreateIssueAttachment (0.49s)
    testlogger.go:61: 2025/09/22 14:12:32 modules/storage/local.go:33:NewLocalStorage() [I] Creating new Local Storage at /home/runner/work/gitea/gitea/tests/gitea-lfs-meta
    testlogger.go:61: 2025/09/22 14:12:32 HTTPRequest [I] router: completed GET /user/login for test-mock:12345, 200 OK in 2.1ms @ auth/auth.go:184(auth.SignIn)
    testlogger.go:61: 2025/09/22 14:12:32 HTTPRequest [I] router: completed POST /user/login for test-mock:12345, 303 See Other in 5.8ms @ auth/auth.go:197(auth.SignInPost)
    testlogger.go:61: 2025/09/22 14:12:32 HTTPRequest [I] router: completed POST /user2/repo1/issues/attachments for test-mock:12345, 500 Internal Server Error in 85.8ms @ repo/attachment.go:25(repo.UploadIssueAttachment)
    attachment_test.go:47: Response:  NewAttachment: attachment size 2147483648 exceed limit 104
        
    attachment_test.go:47: 
        	Error Trace:	/home/runner/work/gitea/gitea/tests/integration/integration_test.go:354
        	            				/home/runner/work/gitea/gitea/tests/integration/integration_test.go:160
        	            				/home/runner/work/gitea/gitea/tests/integration/attachment_test.go:47
        	            				/home/runner/work/gitea/gitea/tests/integration/attachment_test.go:67
        	Error:      	Not equal: 
        	            	expected: 200
        	            	actual  : 500
        	Test:       	TestCreateIssueAttachment
        	Messages:   	Request: POST /user2/repo1/issues/attachments

Also, it's probably better to have a status 413 returned if the size is too large

Signed-off-by: a1012112796 <1012112796@qq.com>

wxiaoguang · 2025-09-23T23:24:40Z

routers/api/v1/repo/release_attachment.go


 	// Create a new attachment and save the file
-	attach, err := attachment_service.UploadAttachment(ctx, content, setting.Repository.Release.AllowedTypes, size, &repo_model.Attachment{
+	attach, err := attachment_service.UploadAttachment(ctx, content, setting.Repository.Release.AllowedTypes, setting.Attachment.MaxSize<<20, size, &repo_model.Attachment{


There are at least 2 "attachment max size" config options.

If you always use setting.Attachment.MaxSize, at least one is wrong

why? looks they are all same in curent design. hasn't found anything like setting.Repository.Release.MaxSize.

ref:

gitea/services/context/upload/upload.go

Lines 90 to 114 in 0b706b0

// AddUploadContext renders template values for dropzone

func AddUploadContext(ctx *context.Context, uploadType string) {

switch uploadType {

case "release":

ctx.Data["UploadUrl"] = ctx.Repo.RepoLink + "/releases/attachments"

ctx.Data["UploadRemoveUrl"] = ctx.Repo.RepoLink + "/releases/attachments/remove"

ctx.Data["UploadLinkUrl"] = ctx.Repo.RepoLink + "/releases/attachments"

ctx.Data["UploadAccepts"] = strings.ReplaceAll(setting.Repository.Release.AllowedTypes, "|", ",")

ctx.Data["UploadMaxFiles"] = setting.Attachment.MaxFiles

ctx.Data["UploadMaxSize"] = setting.Attachment.MaxSize

case "comment":

ctx.Data["UploadUrl"] = ctx.Repo.RepoLink + "/issues/attachments"

ctx.Data["UploadRemoveUrl"] = ctx.Repo.RepoLink + "/issues/attachments/remove"

if len(ctx.PathParam("index")) > 0 {

ctx.Data["UploadLinkUrl"] = ctx.Repo.RepoLink + "/issues/" + url.PathEscape(ctx.PathParam("index")) + "/attachments"

} else {

ctx.Data["UploadLinkUrl"] = ctx.Repo.RepoLink + "/issues/attachments"

}

ctx.Data["UploadAccepts"] = strings.ReplaceAll(setting.Attachment.AllowedTypes, "|", ",")

ctx.Data["UploadMaxFiles"] = setting.Attachment.MaxFiles

ctx.Data["UploadMaxSize"] = setting.Attachment.MaxSize

default:

setting.PanicInDevOrTesting("Invalid upload type: %s", uploadType)

}

}

Oh you are right, release upload shares the same attachment max size with comment attachment, but doesn't share some other settings .....

It seems to be another (unrelated) legacy problem.

Update: the problem I mentioned There are at least 2 "attachment max size" config options. is still there. See my latest comment for UploadFileToServer, there is no limit check either.

fix attachment file size limit in server backend #35519 (comment)

services/attachment/attachment.go

…imit

lunny · 2025-10-04T23:54:37Z

last call @go-gitea/technical-oversight-committee

services/attachment/attachment.go

Co-authored-by: 6543 <6543@obermui.de> Signed-off-by: Lunny Xiao <xiaolunwen@gmail.com>

services/attachment/attachment.go

…size limits

6543 · 2025-10-07T08:26:20Z

@a1012112796 -> a1012112796#13

wxiaoguang · 2025-10-11T11:57:10Z

I try to write a prove of concept

@6543 any proof? If no, revert

@6543 any proof? If no, revert

6543 · 2025-10-11T12:39:25Z

I'll revert it for now as i did not have time to write one jet to test stuff

wxiaoguang · 2025-10-11T12:43:00Z

as i did not have time to write one jet to test stuff

Even if you have 100 years, there will be no proof. Golang HTTP packages's FormFile guarantees that the head.Size is the real uploaded size, you can spend a few minutes to read the code, no need to spend 100 years to try.

6543 · 2025-10-11T16:39:37Z

well did not know/read that docu jet but I should ...
... anyway better save than sorry

wxiaoguang · 2025-10-11T18:14:27Z

There are still problems:

CreateReleaseAttachment: when the body is used as the upload stream, it should use http.MaxBytesReader to limit the size (fixed)
ctx.Req.ParseMultipartForm(setting.Attachment.MaxSize << 20) is not right, it will make Gitea OOM (fixed)
UploadFileToServer doesn't check the size limit either, I just left a FIXME and didn't make more changes in this PR
- Feel free to properly fix it

dismiss

6543

errors.Is dont work now

creating custom errors is fine and all but we should work with golangs error handling system witch means that we should cover unwrap and is interface

wxiaoguang · 2025-10-11T18:29:21Z

errors.Is dont work now (just revert commit looked at jet)

What do you mean by "errors.Is dont work now"?

6543 · 2025-10-11T18:31:41Z

i will have time tomorrow, review via mobule is horrible

wxiaoguang · 2025-10-11T18:33:15Z

creating custom errors is fine and all but we should work with golangs error handling system witch means that we should cover unwrap and is interface

Isn't it what I have done? I don't see any problem.

wxiaoguang · 2025-10-12T17:56:45Z

errors.Is dont work now

What do you mean by "errors.Is dont work now"?

creating custom errors is fine and all but we should work with golangs error handling system witch means that we
should cover unwrap and is interface

Isn't it what I have done? I don't see any problem.

i will have time tomorrow, review via mobule is horrible

@6543 so where is the problem? I think you misunderstood the "golangs error handling system". If no problem, dismiss the change request.

fix attachment file size limit in server backend

7342304

fix go-gitea#35512 Signed-off-by: a1012112796 <1012112796@qq.com>

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 22, 2025

github-actions bot added modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code labels Sep 22, 2025

fix test and apply suggestions

08035a7

Signed-off-by: a1012112796 <1012112796@qq.com>

lunny approved these changes Sep 23, 2025

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Sep 23, 2025

lunny added the type/bug label Sep 23, 2025

wxiaoguang reviewed Sep 23, 2025

View reviewed changes

services/attachment/attachment.go Outdated Show resolved Hide resolved

wxiaoguang reviewed Sep 23, 2025

View reviewed changes

services/attachment/attachment.go Outdated Show resolved Hide resolved

a1012112796 and others added 4 commits September 24, 2025 22:19

Merge remote-tracking branch 'origin/main' into zzc/dev/attach_size_l…

d11f7c8

…imit

fix nits

e19a4b1

user modern golang error system

5f200ee

Merge branch 'main' into zzc/dev/attach_size_limit

f8c7971

Merge branch 'main' into zzc/dev/attach_size_limit

38ce392

6543 reviewed Oct 6, 2025

View reviewed changes

services/attachment/attachment.go Outdated Show resolved Hide resolved

Update services/attachment/attachment.go

f040898

Co-authored-by: 6543 <6543@obermui.de> Signed-off-by: Lunny Xiao <xiaolunwen@gmail.com>

lunny added this to the 1.26.0 milestone Oct 6, 2025

lunny added the backport/v1.25 label Oct 6, 2025

6543 reviewed Oct 6, 2025

View reviewed changes

services/attachment/attachment.go Outdated Show resolved Hide resolved

6543 added 2 commits October 7, 2025 09:30

enhance test TestUploadAttachment

c90df6d

introduce attachmentLimitedReader to make sure io stream dont bypass …

a70dc88

…size limits

6543 added 3 commits October 7, 2025 10:27

Update services/attachment/reader.go

7316593

Update services/attachment/reader.go

9e16482

Update services/attachment/reader.go

9c1e0b5

6543 added 2 commits October 11, 2025 18:40

Merge branch 'main' into zzc/dev/attach_size_limit

f7107de

no attachmentLimitedReader

5f39a09

6543 requested a review from wxiaoguang October 11, 2025 16:45

wxiaoguang added 3 commits October 12, 2025 00:49

revert

caceb5e

fix

63602dc

add fixme

38e4cb3

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Oct 11, 2025

wxiaoguang requested a review from 6543 October 11, 2025 18:14

GiteaBot removed the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Oct 11, 2025

wxiaoguang requested a review from lunny October 11, 2025 18:14

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 11, 2025

wxiaoguang added 2 commits October 12, 2025 02:18

fix

d8c61f3

fix pkg import

b59f439

6543 requested changes Oct 11, 2025

View reviewed changes

GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 11, 2025

lunny approved these changes Oct 11, 2025

View reviewed changes

	// AddUploadContext renders template values for dropzone
	func AddUploadContext(ctx *context.Context, uploadType string) {
	switch uploadType {
	case "release":
	ctx.Data["UploadUrl"] = ctx.Repo.RepoLink + "/releases/attachments"
	ctx.Data["UploadRemoveUrl"] = ctx.Repo.RepoLink + "/releases/attachments/remove"
	ctx.Data["UploadLinkUrl"] = ctx.Repo.RepoLink + "/releases/attachments"
	ctx.Data["UploadAccepts"] = strings.ReplaceAll(setting.Repository.Release.AllowedTypes, "\|", ",")
	ctx.Data["UploadMaxFiles"] = setting.Attachment.MaxFiles
	ctx.Data["UploadMaxSize"] = setting.Attachment.MaxSize
	case "comment":
	ctx.Data["UploadUrl"] = ctx.Repo.RepoLink + "/issues/attachments"
	ctx.Data["UploadRemoveUrl"] = ctx.Repo.RepoLink + "/issues/attachments/remove"
	if len(ctx.PathParam("index")) > 0 {
	ctx.Data["UploadLinkUrl"] = ctx.Repo.RepoLink + "/issues/" + url.PathEscape(ctx.PathParam("index")) + "/attachments"
	} else {
	ctx.Data["UploadLinkUrl"] = ctx.Repo.RepoLink + "/issues/attachments"
	}
	ctx.Data["UploadAccepts"] = strings.ReplaceAll(setting.Attachment.AllowedTypes, "\|", ",")
	ctx.Data["UploadMaxFiles"] = setting.Attachment.MaxFiles
	ctx.Data["UploadMaxSize"] = setting.Attachment.MaxSize
	default:
	setting.PanicInDevOrTesting("Invalid upload type: %s", uploadType)
	}
	}

Uh oh!

fix attachment file size limit in server backend #35519

Are you sure you want to change the base?

fix attachment file size limit in server backend #35519

Conversation

a1012112796 commented Sep 22, 2025

Uh oh!

techknowlogick commented Sep 22, 2025

Uh oh!

wxiaoguang Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

a1012112796 Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

wxiaoguang Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

wxiaoguang Oct 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lunny commented Oct 4, 2025

Uh oh!

Uh oh!

Uh oh!

6543 commented Oct 7, 2025

Uh oh!

wxiaoguang commented Oct 11, 2025

Uh oh!

6543 commented Oct 11, 2025

Uh oh!

wxiaoguang commented Oct 11, 2025

Uh oh!

6543 commented Oct 11, 2025

Uh oh!

wxiaoguang commented Oct 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

6543 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wxiaoguang commented Oct 11, 2025

Uh oh!

6543 commented Oct 11, 2025

Uh oh!

wxiaoguang commented Oct 11, 2025

Uh oh!

wxiaoguang commented Oct 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

wxiaoguang Oct 11, 2025 •

edited

Loading

wxiaoguang commented Oct 11, 2025 •

edited

Loading

6543 left a comment •

edited

Loading